@slack/web-api: prep for next major release. bump min node to v18 #1667
Conversation
…eaning up the readme. remove no longer used codecov and deno build npm run scripts. update dependencies as much as possible. fix linter errors. remove a test that is no longer applicable with new axios.
| @@ -34,20 +34,6 @@ describe('WebClient', function () { | |||
| assert.instanceOf(client, WebClient); | |||
| assert.notExists(client.axios.defaults.headers.Authorization); | |||
| }); | |||
| it('should not modify global defaults in axios', function () { | |||
There was a problem hiding this comment.
I didn't feel like this test offers much value and since this PR moves from axios 0.x to 1.x, also not sure how much we should be testing the output of other packages.
There was a problem hiding this comment.
@filmaj As you can see, the context behind this unit test is #1037 This package had an issue polluting the global singleton instance of axios package. This test tries to prevent the same issue from occurring again. If axois 1.x does not allow such destructive operations to the global singleton, we can safely remove this test. Thus, I don't think we're testing 3rd party library here. We verify our package works without any major side effects.
If investigation on how to properly maintain this test with axios 1.x could require our time and efforts, it's fine to delete the test anyways. Whenever we have something similar in the future, we can add tests again.
There was a problem hiding this comment.
It seems in the latest axios, no specific value is set for the HTTP POST default headers. This is also the case for 'common' HTTP all-verb headers.
In v0.27.2 of axios, this was not the case: POST would set a default Content-Type. and the default was application/x-www-form-urlencoded.
Therefore, I think this test can be safely removed.
| @@ -149,7 +135,6 @@ describe('WebClient', function () { | |||
| assert.equal(error.code, ErrorCode.RequestError); | |||
| assert.equal(error.original.config.timeout, timeoutOverride); | |||
| assert.equal(error.original.isAxiosError, true); | |||
| assert.equal(error.original.request.aborted, true); | |||
There was a problem hiding this comment.
In axios 1.0, this assertion no longer passes. Not sure how important that is. Let me know if you have opinions on that.
There was a problem hiding this comment.
I don't see any issues with the deletion
| @@ -34,20 +34,6 @@ describe('WebClient', function () { | |||
| assert.instanceOf(client, WebClient); | |||
| assert.notExists(client.axios.defaults.headers.Authorization); | |||
| }); | |||
| it('should not modify global defaults in axios', function () { | |||
There was a problem hiding this comment.
@filmaj As you can see, the context behind this unit test is #1037 This package had an issue polluting the global singleton instance of axios package. This test tries to prevent the same issue from occurring again. If axois 1.x does not allow such destructive operations to the global singleton, we can safely remove this test. Thus, I don't think we're testing 3rd party library here. We verify our package works without any major side effects.
If investigation on how to properly maintain this test with axios 1.x could require our time and efforts, it's fine to delete the test anyways. Whenever we have something similar in the future, we can add tests again.
| @@ -149,7 +135,6 @@ describe('WebClient', function () { | |||
| assert.equal(error.code, ErrorCode.RequestError); | |||
| assert.equal(error.original.config.timeout, timeoutOverride); | |||
| assert.equal(error.original.isAxiosError, true); | |||
| assert.equal(error.original.request.aborted, true); | |||
There was a problem hiding this comment.
I don't see any issues with the deletion
| @@ -94,7 +94,13 @@ export function httpErrorFromResponse(response: AxiosResponse): WebAPIHTTPError | |||
| ) as Partial<WebAPIHTTPError>; | |||
| error.statusCode = response.status; | |||
| error.statusMessage = response.statusText; | |||
| error.headers = response.headers; | |||
| const nonNullHeaders: Record<string, string> = {}; | |||
There was a problem hiding this comment.
TypeScript made me do it 😄
| * [Email us](mailto:[email protected]) in Slack developer support: `[email protected]` | ||
| * [Bot Developers Hangout](https://community.botkit.ai/): a Slack community for developers | ||
| building all types of bots. You can find the maintainers and users of these packages in **#sdk-node-slack-sdk**. | ||
| * [Email us](mailto:[email protected]): `[email protected]` |
There was a problem hiding this comment.
Should they email [email protected] or [email protected]? other repos tell developers to email [email protected]
There was a problem hiding this comment.
My understanding is [email protected]
Summary
I'd like to work on slowly getting the next 7.0 major release out, and this would be a good opportunity to eliminate EOL'ed node support while we are at it. I will be reviewing the existing issues for this package to see if we can tackle any other breaking changes while we are at it.